Bikeshed: I think I prefer do_not_serialize as a name here.

done!

More bike shedding 😄: no_serde or skip_serde.

It's just a bit less to write and cleaner. But do_not_serialize isn't bad either haha

Edit: or use skip_serializing like serde does

Hmm, I am against serde mentions since this might make people think (more than it does now) this applies serde's skip_serializing to the field which it doesn't.

Right now this only disables automatic Reflect serialization.

Edit: skip_serializing does sound good too, which one should I go for ?

skip_serializing for consistency with serde is my new favorite 😄

This probably needs to be rebased. @makspll would you be able to do that?

I am currently on holiday, but can get on this ~monday

Aight, so I am working on this now, it's been a while so I'll take my time with this (and reflection changed a lot by the looks of it) but one thing I noticed so far:
when running the scene example,

bevy_asset::asset_server: encountered an error while loading an asset: No registration found for `glam::vec3::Vec3`

Now I can't remember if that was there before, probably was but I didn't notice, what I think is happening is that because of the way glam imports changed, the type names are now platform dependent, meaning that on my machine the type name this is saved under is now: glam::f32::vec3::Vec3, this is not a problem with this PR in that case and it might have already been addressed somewhere else (and might be fixed after I re-base) but I am just gonna leave this mental note here.

I've squashed all the commits for ease of re-basing, apologies for the force push

Now I can't remember if that was there before, probably was but I didn't notice, what I think is happening is that because of the way glam imports changed, the type names are now platform dependent, meaning that on my machine the type name this is saved under is now: glam::f32::vec3::Vec3, this is not a problem with this PR in that case and it might have already been addressed somewhere else (and might be fixed after I re-base) but I am just gonna leave this mental note here.

Yep this is a known issue and we're trying to fix it via #5805.

Yep this is a known issue and we're trying to fix it via #5805.

Okay that's fantastic, I remember bumping into that problem earlier with scripting but it completely fell out my head

One question @MrGVSV, are enum variants supposed to be ignorable by reflection/serialization ?

If so, how would the serialziation/deserialization of ignored variants look like?

One question @MrGVSV, are enum variants supposed to be ignorable by reflection/serialization ?

I added it as an option. I don't know what specific use-cases it has, though. My original thought was that it would allow for enums to contain variants meant for runtime data and variants meant for serialized data, such as HandleId.

If you find that it's not necessary or should be removed, feel free to make an issue for it, though! I have no problem removing it if it isn't useful haha

One question @MrGVSV, are enum variants supposed to be ignorable by reflection/serialization ?

I added it as an option. I don't know what specific use-cases it has, though. My original thought was that it would allow for enums to contain variants meant for runtime data and variants meant for serialized data, such as HandleId.

If you find that it's not necessary or should be removed, feel free to make an issue for it, though! I have no problem removing it if it isn't useful haha

Hmm I see, Yeah I definitely see possible uses for it so removing is not what I want to do. I am wondering however if the current reflect(ignore) works at all (unless I am missing something, and I very well might be, I just didn't see tests for this)? Since the deserialization requires a variant field, which one can only access at serialization IFF the variant is not ignored (the macros don't process inactive variants at all). The solution is probably modifying the macros to generate variant names for all variants regardless if they are ignored.

First bit of serialization:

        let variant_type = self.enum_value.variant_type(); // crash here on ignored variants
        let variant_name = self.enum_value.variant_name();

First bit of deserialization:

        let key = map.next_key::<String>()?;
        match key.as_deref() {
            Some(type_fields::VARIANT) => {}
            Some(key) => return Err(V::Error::unknown_field(key, &[type_fields::VARIANT])),
            _ => {
                return Err(V::Error::missing_field(type_fields::VARIANT));
            }
        }

Relevant macro code on main:

  for variant in reflect_enum.active_variants() {
        let ident = &variant.data.ident;
        let name = ident.to_string();
        let unit = reflect_enum.get_unit(ident);

       // ... more code ...

        let mut add_fields_branch = |variant, info_type, arguments, field_len| {
            // .. more code ..
            // note this code doesn't run for ignored variants
            enum_field_len.push(quote! {
                #unit{..} => #field_len
            });
            enum_variant_name.push(quote! {
                #unit{..} => #name
            });
            enum_variant_type.push(quote! {
                #unit{..} => #bevy_reflect_path::VariantType::#variant
            });
        };

Hmm I see, Yeah I definitely see possible uses for it so removing is not what I want to do. I am wondering however if the current reflect(ignore) works at all (unless I am missing something, and I very well might be, I just didn't see tests for this)? Since the deserialization requires a variant field, which one can only access at serialization IFF the variant is not ignored (the macros don't process inactive variants at all). The solution is probably modifying the macros to generate variant names for all variants regardless if they are ignored.

Oh great catch! Yeah, it's a bit weird because methods like Enum::variant_type work directly on the type, making it difficult to partially ignore. I see three options:

1. Do like you suggest, and just don't ignore the variants for any method that relies on current variant info
2. Have those methods return sensible defaults rather than throwing unreachable!() (e.g. return Option<&str> rather than &str)
3. Do not allow variants to be ignored

While I see the possible usefulness of ignoring variants, perhaps it's better to just go with Option 3 and remove this functionality altogether (at least for the time being). Option 1 isn't awful but I think it will have a breadth that basically matches Option 3. Option 2 just hurts ergonomics imo.

I think it will be better to remove this as it's a really exposed footgun (oversight on my part). We can always add it back in, and it would be better to remove it now before 0.9 releases with it (forcing a possible future removal to be a breaking change).

I'll make an issue for this unless there's any pushback.

@MrGVSV yeah okay, I haven't touched much code in a month so I assumed I was tripping :P

Alright well, In that case I am in favour of removing the variant ignore behaviour for now on the basis of what's already been said + the fact that adding it complicates this PR a bit:

without variant serialization, I can represent "Serialization Ignoring Behaviour" with a set of field indices, with variants I now need an Enum, with either that or a set of strings for variant names. Keeping just index sets is probably more performant and ergonomic, since the serialization process is just:

for (idx,field) in my_thing.iter_fields().enumerate(){
  if !ignored.contains(idx){
     // serialize 
  }
}

Alright so that should be it then, I've removed the variant ignoring mechanisms, and re-based everything, should be ready for review

Alrighty, that should be @MrGVSV's review implemented, biggest change is SerializationData is now under data in the registry

Fantastic,
This PR will of course need a follow up updating the existing #[reflect(ignore)] and swapping to #[reflect(skip_serializing)] where possible, I am happy to do that after this PR is merged

bors r+

Build failed (retrying...):

• build-and-install-on-iOS

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12